Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AWS Glacier/Deep Archive loading of restored objects #759

Merged
merged 5 commits into from
Feb 20, 2020

Conversation

brainstorm
Copy link
Contributor

@brainstorm brainstorm commented Feb 18, 2020

Up until now, error messages with AWS Glacier were limited to knowing the storage class.

When an object gets "thawed" (restored) from AWS Glacier, it holds the StorageClass (it does not transition to i.e standard). Instead, the header x-amz-restore should be parsed accordingly for the different lifecycle stages where the S3 object might be in.

While I was at it, I also added support for directly loading those S3 objects from Load-> From URL menu, to be consistent in UX/behaviour with the S3 Tree selector... this is a very germane change and goes along with this same PR!

Please @igvteam @jrobinso @davideby, would you please expedite this PR merge and release? Otherwise the Glacier object restoration will remain inoperative and confusing to users :-S

/cc @reisingerf @andrewpatto

… header combinations while debugging do not seem to align docs, more investigation is needed.
…mented support for File->Load From URL if the user knows the S3 URL, they can paste it there directly. Also aws-java-sdk-v2 version bump from Maven
…ion bump since it affects error messaging and "germane-ity" of this PR. Killing Triple/Tuple in next commit.
@brainstorm
Copy link
Contributor Author

brainstorm commented Feb 19, 2020

@jrobinso Sorry for the last commit, no more are coming in unless you have something to add here.

This should be ready to merge and release, imho ;)

AmazonUtils.s3ObjectAccessResult res = isObjectAccessible(bucket, key);
if (!res.getObjAvailable()) { MessageUtils.showErrorMessage(res.getErrorReason(), null); return; }
}
} catch (NullPointerException npe) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment, do we know that the user hasn't logged in solely from the npe? Could there be other reasons for an npe?

@jrobinso jrobinso merged commit 4fbb9df into igvteam:master Feb 20, 2020
@brainstorm brainstorm deleted the fix_glacier_loading branch February 20, 2020 23:43
@brainstorm
Copy link
Contributor Author

Thanks for merging that fast, Jim! Looking forward for next release so I can tell our users to update.

I had exactly the same thought about that try-catch, but when I started to implement a isAWSAuthenticated() type of logic to make it better, it went down to a chicken-and-egg rabbit hole of auth states, so I don't want to complicate that bit, if you don't mind.

Promise I'll fix/refactor that part if it becomes a problem ;)

@jrobinso
Copy link
Contributor

I don't mind, just noting it, sometimes the simple solution is best. Appreciate the PR.

@jrobinso
Copy link
Contributor

Oh on release, I have a juicebox.js and igv.js release to get out first, then we can look at releasing this. Maybe end of next week or week after, ping me if it drags on past that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants